[Explore] Altered Slice Tag #3668

Mogball · 2017-10-13T18:05:37Z

Based on a suggesting by @mistercrunch in #3617, this PR implements an AlteredSliceTag which is displayed next to the slice title in the explore view when the current controls do not match the saved slice controls:

Users can click the tag to open a modal summarizing the differences:

Most importantly, exploring a slice from a dashboard with dashboard filters will show the slice as altered with the applied filters as differences.

Also fixes #3616

coveralls · 2017-10-13T18:25:03Z

Coverage increased (+0.06%) to 70.447% when pulling c1ff26f on Mogball:mogball/feature/altered_explore_slice into bad6938 on apache:master.

coveralls · 2017-10-13T18:25:03Z

Coverage increased (+0.06%) to 70.447% when pulling c1ff26f on Mogball:mogball/feature/altered_explore_slice into bad6938 on apache:master.

mistercrunch

Looking forward to this feature!

mistercrunch · 2017-10-16T23:48:58Z

superset/assets/javascripts/explore/components/ChartContainer.jsx

@@ -145,6 +146,25 @@ class ChartContainer extends React.PureComponent {
    };
  }

+  isAltered() {


What about using a deep equality function like http://underscorejs.org/#isEqual instead?

mistercrunch · 2017-10-16T23:52:12Z

superset/assets/javascripts/components/AlteredSliceTag.jsx

+  }
+}
+
+AlteredSliceTag.propTypes = {


Please put propTypes at the top of the file as in most other modules

Also what about if the component received origFormData and currentFormData instead, and the logic to check the differences can live in this component here.

mistercrunch · 2017-10-16T23:55:23Z

superset/assets/javascripts/explore/components/ChartContainer.jsx

+        differing[fdKey] = { before: bfd[fdKey], after: fd[fdKey] };
+      }
+    }
+    return differing;


I'd expect a isSomething method to return a bool by naming conventions, rename the function to something like getDiffs.

mistercrunch · 2017-10-16T23:57:49Z

superset/assets/javascripts/components/AlteredSliceTag.jsx

+    } else if (value.constructor === Object) {
+      return JSON.stringify(value);
+    }
+    return value;


I'm thinking the catchall could be a JSON.stringify though I'm not 100 sure of the implications. There's a guarantee that controls' values are always serializable since they make it through the redux store.

mistercrunch · 2017-10-17T00:01:07Z

superset/assets/javascripts/explore/components/ChartContainer.jsx

+    // Returns all properties that differ in the
+    // current form data and the base form data
+    const fd = this.props.formData || {};
+    const bfd = (this.props.slice && this.props.slice.form_data) || {};


Here you may want to use the currentFormData as the driver of which keys should exist. slice.form_data may have unused / deprecated keys that don't affect how the slice is rendered...

Mogball · 2017-10-17T17:46:45Z

Putting getDiffs in AlteredSliceTag means we can unit test it as well!

coveralls · 2017-10-18T01:47:46Z

Coverage increased (+0.06%) to 70.819% when pulling 16f175767899dc6af24b5a28bcecf36e6b69abbe on Mogball:mogball/feature/altered_explore_slice into e121a85 on apache:master.

coveralls · 2017-10-18T01:47:47Z

Coverage increased (+0.06%) to 70.819% when pulling 16f175767899dc6af24b5a28bcecf36e6b69abbe on Mogball:mogball/feature/altered_explore_slice into e121a85 on apache:master.

coveralls · 2017-11-01T18:18:15Z

Coverage increased (+0.06%) to 71.332% when pulling 2e83588 on Mogball:mogball/feature/altered_explore_slice into abfa034 on apache:master.

coveralls · 2017-11-01T18:18:15Z

Coverage increased (+0.06%) to 71.332% when pulling 2e83588 on Mogball:mogball/feature/altered_explore_slice into abfa034 on apache:master.

coveralls · 2017-11-01T18:18:15Z

Coverage increased (+0.06%) to 71.332% when pulling 2e83588 on Mogball:mogball/feature/altered_explore_slice into abfa034 on apache:master.

mistercrunch · 2017-11-10T04:43:10Z

Ok this is ready to merge upon fixing conflicts.

coveralls · 2017-11-10T07:21:44Z

Coverage increased (+0.06%) to 71.517% when pulling 5301b10 on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:29:09Z

Coverage decreased (-0.08%) to 71.376% when pulling e1ba9c50b481ee521d3715581cb5239f4f7bee14 on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:46:38Z

Coverage increased (+0.06%) to 71.517% when pulling c364a403ac639aa73b74e124488544aec980339c on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:46:38Z

Coverage increased (+0.06%) to 71.517% when pulling c364a403ac639aa73b74e124488544aec980339c on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:46:38Z

Coverage increased (+0.06%) to 71.517% when pulling c364a403ac639aa73b74e124488544aec980339c on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:46:39Z

Coverage increased (+0.06%) to 71.517% when pulling c364a403ac639aa73b74e124488544aec980339c on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:59:17Z

Coverage increased (+0.06%) to 71.517% when pulling c7a3c49 on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:59:17Z

Coverage increased (+0.06%) to 71.517% when pulling c7a3c49 on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

coveralls · 2017-11-10T07:59:17Z

Coverage increased (+0.06%) to 71.517% when pulling c7a3c49 on Mogball:mogball/feature/altered_explore_slice into 6c52f2f on apache:master.

Mogball · 2017-11-10T18:59:43Z

@mistercrunch done

* Added altered tag to explore slice view and fixes apache#3616 * unit tests * Moved getDiffs logic into AlteredSliceTag * code style fixs

Mogball added 2 commits October 12, 2017 23:13

Added altered tag to explore slice view and fixes apache#3616

b4db769

unit tests

c1ff26f

mistercrunch requested changes Oct 17, 2017

View reviewed changes

Mogball force-pushed the mogball/feature/altered_explore_slice branch from 28a3fea to 5154c0a Compare October 17, 2017 17:52

Mogball closed this Oct 18, 2017

Mogball reopened this Oct 18, 2017

Mogball force-pushed the mogball/feature/altered_explore_slice branch from 16f1757 to 5154c0a Compare October 30, 2017 06:31

Mogball closed this Oct 31, 2017

Mogball reopened this Oct 31, 2017

Mogball force-pushed the mogball/feature/altered_explore_slice branch from 31a9d5e to 5154c0a Compare October 31, 2017 06:50

Moved getDiffs logic into AlteredSliceTag

2e83588

Mogball force-pushed the mogball/feature/altered_explore_slice branch from 09f333c to 2e83588 Compare November 1, 2017 18:00

mistercrunch approved these changes Nov 10, 2017

View reviewed changes

mistercrunch mentioned this pull request Nov 10, 2017

[cache] Fixing cache key w/ merged extra filters #3809

Merged

Merge branch 'master' into mogball/feature/altered_explore_slice

5301b10

Mogball force-pushed the mogball/feature/altered_explore_slice branch from e1ba9c5 to c364a40 Compare November 10, 2017 07:31

code style fixs

c7a3c49

Mogball force-pushed the mogball/feature/altered_explore_slice branch from c364a40 to c7a3c49 Compare November 10, 2017 07:42

mistercrunch merged commit 4d48d5d into apache:master Nov 11, 2017

nbonnotte mentioned this pull request Dec 18, 2017

Table View: unsupported operand type(s) for +: 'NoneType' and 'str' #4042

Closed

3 tasks

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

[Explore] Altered Slice Tag (apache#3668)

9f7736b

* Added altered tag to explore slice view and fixes apache#3616 * unit tests * Moved getDiffs logic into AlteredSliceTag * code style fixs

snyk-bot mentioned this pull request Aug 19, 2021

[Snyk] Security upgrade webpack-dev-server from 3.11.0 to 4.0.0 XirdigH/superset#25

Closed

m-ajay mentioned this pull request Sep 27, 2022

[Snyk] Fix for 9 vulnerabilities m-ajay/superset#30

Open

This was referenced Oct 6, 2022

[Snyk] Fix for 9 vulnerabilities m-ajay/superset#53

Open

[Snyk] Fix for 9 vulnerabilities m-ajay/superset#62

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0 labels Feb 27, 2024

m-ajay mentioned this pull request Mar 15, 2024

[Snyk] Fix for 2 vulnerabilities m-ajay/superset#271

Open

m-ajay mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities m-ajay/superset#278

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Explore] Altered Slice Tag #3668

[Explore] Altered Slice Tag #3668

Mogball commented Oct 13, 2017

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017 •

edited

Loading

mistercrunch left a comment

mistercrunch Oct 16, 2017

mistercrunch Oct 16, 2017

mistercrunch Oct 16, 2017

mistercrunch Oct 16, 2017

mistercrunch Oct 16, 2017

mistercrunch Oct 17, 2017

Mogball commented Oct 17, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Nov 1, 2017 •

edited

Loading

coveralls commented Nov 1, 2017

coveralls commented Nov 1, 2017

mistercrunch commented Nov 10, 2017

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017

Mogball commented Nov 10, 2017

[Explore] Altered Slice Tag #3668

[Explore] Altered Slice Tag #3668

Conversation

Mogball commented Oct 13, 2017

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017 • edited Loading

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch Oct 16, 2017

Choose a reason for hiding this comment

mistercrunch Oct 16, 2017

Choose a reason for hiding this comment

mistercrunch Oct 16, 2017

Choose a reason for hiding this comment

mistercrunch Oct 16, 2017

Choose a reason for hiding this comment

mistercrunch Oct 16, 2017

Choose a reason for hiding this comment

mistercrunch Oct 17, 2017

Choose a reason for hiding this comment

Mogball commented Oct 17, 2017

coveralls commented Oct 18, 2017

coveralls commented Oct 18, 2017 • edited Loading

coveralls commented Nov 1, 2017 • edited Loading

coveralls commented Nov 1, 2017

coveralls commented Nov 1, 2017

mistercrunch commented Nov 10, 2017

coveralls commented Nov 10, 2017 • edited Loading

coveralls commented Nov 10, 2017 • edited Loading

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017

coveralls commented Nov 10, 2017 • edited Loading

coveralls commented Nov 10, 2017

Mogball commented Nov 10, 2017

coveralls commented Oct 13, 2017 •

edited

Loading

coveralls commented Oct 18, 2017 •

edited

Loading

coveralls commented Nov 1, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading

coveralls commented Nov 10, 2017 •

edited

Loading